-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix keepRuntimeAlive in the case where EXIT_RUNTIME=0 and noExitRuntime is not referenced #22542
base: main
Are you sure you want to change the base?
Conversation
47833bf
to
0fed818
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the test to avoid the sleep. PTAL
0fed818
to
d88a5a2
Compare
src/library_pthread.js
Outdated
@@ -188,7 +188,7 @@ var LibraryPThread = { | |||
// worker pool as an unused worker. | |||
worker.pthread_ptr = 0; | |||
|
|||
#if ENVIRONMENT_MAY_BE_NODE && PROXY_TO_PTHREAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify why this change is necessary? AFAIK, all threads should already be weakly referenced by this point, this should only be required for the "main" thread in PROXY_TO_PTHREAD
mode, see:
#19073 (comment)
63d5d7a
to
5f573a9
Compare
…me is not referenced Essentially the `keepRuntimeAlive` was relying on the `noExitRuntime` variable being set based on `EXIT_RUNTIME` but when `noExitRuntime` was absent the `EXIT_RUNTIME` settings was being ignored and the runtime was exiting even though `EXIT_RUNTIME=0` was set (the default). Fixes: emscripten-core#20636
5f573a9
to
6df94dd
Compare
int keep_alive = EM_ASM_INT({ | ||
setTimeout(timeout_func); | ||
return keepRuntimeAlive(); | ||
}, emscripten_force_exit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a parameter passed in here?
// EXIT_RUNTIME=0). | ||
|
||
EM_JS(void, timeout_func, (), { | ||
console.log("timeout_func: keepRuntimeAlive() ->", keepRuntimeAlive()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you not need to ensure that keepRuntimeAlive is available in JS?
Essentially the
keepRuntimeAlive
was relying on thenoExitRuntime
variable being set based onEXIT_RUNTIME
but whennoExitRuntime
was absent theEXIT_RUNTIME
settings was being ignored and the runtime was exiting even thoughEXIT_RUNTIME=0
was set (the default).Fixes: #20636